ENH: Add lookup and putting of private elements #508
Conversation
There was a problem hiding this comment.
Hey @naterichman, I made a quick code review, but it seems the branch also needs a rebase.
|
I’ll rebase. I could have sworn I checkout from the master branch. I’m still getting used to contributing to a repo that’s not my own on GitHub (use gitlab for work) Anyway I was thinking, regarding your comment about the attribute selector API on #431 . Do you want to keep methods like this and add supporting methods for the attribute selector API or would you be more interested in merging them into one single API for private tag access? I was thinking we could expand the |
It's OK, it's easy to miss the fact that one has to do both
We can keep them separate. Each API usually stands on its own, meaning that one can do most of the things without the operations API. In other words, we can start with what we have in this PR, which is provide these methods to the in-mem DICOM object impl.
I'm not entirely sure how that would look like, but I suggest we defer to a separate PR. If #508 is cleaned up and made ready within the next day or two, it might just be ready in time for the upcoming major release. |
* Format object/src/mem.rs and object/src/lib.rs * Change from u16 to u8, removing one error type
naterichman
force-pushed
the
private-tag-obj
branch
from
21d1790 to
3d7e3b2
Compare
|
Most of that is a big il’ facepalm. I’ll get this done today! |
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
* Add docs for new public functions * Make the PrivateElementError enum represent tags as hex * Add some tests for error representation
- tweak phrasing and expand error conditions - adjust doctests to be more idiomatic
There was a problem hiding this comment.
Just added a few minor tweaks to the documentation in the last few commits.
The method signatures are good and the tests seem to cover the intended use cases well. Future work will hopefully tackle a more generalised awareness of private elements in the rest of the ecosystem, but it's great to have this PR as the first step towards it. Much appreciated! 👍
Fairly simple lookup and adding of private tags with creator for
InMemDicomObjectas briefly discussed in #431